Skip to content

Conversation

@shiltian
Copy link
Contributor

No description provided.

@shiltian shiltian marked this pull request as ready for review September 13, 2024 13:26
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @shiltian and the rest of your teammates on Graphite Graphite

@llvmbot
Copy link
Member

llvmbot commented Sep 13, 2024

@llvm/pr-subscribers-backend-amdgpu

Author: Shilei Tian (shiltian)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/108560.diff

1 Files Affected:

  • (modified) llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp (+16-13)
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp b/llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
index 687a7339da379d..37c8b043aca198 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
@@ -1077,19 +1077,22 @@ static bool runImpl(Module &M, AnalysisGetter &AG, TargetMachine &TM,
       addPreloadKernArgHint(*F, TM);
     }
 
-    for (auto &I : instructions(F)) {
-      if (auto *LI = dyn_cast<LoadInst>(&I)) {
-        A.getOrCreateAAFor<AAAddressSpace>(
-            IRPosition::value(*LI->getPointerOperand()));
-      } else if (auto *SI = dyn_cast<StoreInst>(&I)) {
-        A.getOrCreateAAFor<AAAddressSpace>(
-            IRPosition::value(*SI->getPointerOperand()));
-      } else if (auto *RMW = dyn_cast<AtomicRMWInst>(&I)) {
-        A.getOrCreateAAFor<AAAddressSpace>(
-            IRPosition::value(*RMW->getPointerOperand()));
-      } else if (auto *CmpX = dyn_cast<AtomicCmpXchgInst>(&I)) {
-        A.getOrCreateAAFor<AAAddressSpace>(
-            IRPosition::value(*CmpX->getPointerOperand()));
+    // Don't bother to run AAAddressSpace for graphics.
+    if (!AMDGPU::isGraphics(F->getCallingConv())) {
+      for (auto &I : instructions(F)) {
+        if (auto *LI = dyn_cast<LoadInst>(&I)) {
+          A.getOrCreateAAFor<AAAddressSpace>(
+              IRPosition::value(*LI->getPointerOperand()));
+        } else if (auto *SI = dyn_cast<StoreInst>(&I)) {
+          A.getOrCreateAAFor<AAAddressSpace>(
+              IRPosition::value(*SI->getPointerOperand()));
+        } else if (auto *RMW = dyn_cast<AtomicRMWInst>(&I)) {
+          A.getOrCreateAAFor<AAAddressSpace>(
+              IRPosition::value(*RMW->getPointerOperand()));
+        } else if (auto *CmpX = dyn_cast<AtomicCmpXchgInst>(&I)) {
+          A.getOrCreateAAFor<AAAddressSpace>(
+              IRPosition::value(*CmpX->getPointerOperand()));
+        }
       }
     }
   }

@arsenm
Copy link
Contributor

arsenm commented Sep 13, 2024

No test and I wouldn't bother skipping it? Do compute shaders really not have flat addresses?

@shiltian
Copy link
Contributor Author

No test and I wouldn't bother skipping it? Do compute shaders really not have flat addresses?

That's what you said:

// Don't bother running InferAddressSpaces pass on graphics shaders which

@shiltian
Copy link
Contributor Author

I wouldn't bother skipping it

If we don't want to bother skipping it, do we want to do the same thing for InferAddressSpacePass? This can allow the TTI version of getFlatAddressSpace to go.

@arsenm
Copy link
Contributor

arsenm commented Sep 13, 2024

I wouldn't bother skipping it

If we don't want to bother skipping it, do we want to do the same thing for InferAddressSpacePass? This can allow the TTI version of getFlatAddressSpace to go.

I'm pretty sure that field is just broken and should be removed. It is initialized by whatever function happens to be first when constructing the TTIImpl. The TTI is owned by the subtarget, which is unique per subtarget, not function

@arsenm arsenm requested review from Flakebi, jayfoad and rovka September 13, 2024 18:26
@arsenm
Copy link
Contributor

arsenm commented Sep 13, 2024

I'm pretty sure that field is just broken and should be removed. It is initialized by whatever function happens to be first when constructing the TTIImpl. The TTI is owned by the subtarget, which is unique per subtarget, not function

Maybe this is wrong, there are other function dependent fields here

@shiltian
Copy link
Contributor Author

I'm pretty sure that field is just broken and should be removed. It is initialized by whatever function happens to be first when constructing the TTIImpl. The TTI is owned by the subtarget, which is unique per subtarget, not function

Maybe this is wrong, there are other function dependent fields here

It is per-function, which is created by a target dependent approach, such as GCNTargetMachine::getTargetTransformInfo.

@shiltian
Copy link
Contributor Author

This is handled directly in AAAddressSpaceInfo, as shown in #108713, so this PR is no longer needed.

@shiltian shiltian closed this Sep 14, 2024
@shiltian shiltian deleted the users/shiltian/no-aa-as-for-gfx branch September 14, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants